-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tf_prefix param: add to IMU Broadcaster and fix slashes in DiffDrive #1104
base: master
Are you sure you want to change the base?
Conversation
@saikishor FYI |
Hi @christophfroehlich @saikishor, |
std::string tf_prefix = ""; | ||
if (!params_.tf_frame_prefix.empty()) | ||
{ | ||
tf_prefix = params_.tf_frame_prefix; | ||
} | ||
else | ||
{ | ||
tf_prefix = std::string(get_node()->get_namespace()); | ||
if (tf_prefix != "/") | ||
{ | ||
tf_prefix += '/'; | ||
} | ||
} | ||
if (tf_prefix.front() == '/') | ||
{ | ||
tf_prefix.erase(0, 1); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello!
While reviewing this part again, I started to question myself, if we need this or not. I'm saying because in case of the diff_drive_controller, the odom
message that is being published is a dynamic tf frame, but in case of the imu_state_broadcaster, as per the imu you always have a static transform link inside the URDF, I don't know if it make sense to have the tf_prefix
in this case.
Right now, your changes will add namespace of the controller manager to the links, I'm not sure how right is it.
Does it make sense?. Should we check if the link exists from the loaded URDF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @saikishor,
Thank you, for reply!
I'm not sure what difference it makes whether fame is dynamic or static in the case of tf_prefix. I treat it as an option that may be useful to someone. What I care about most is the ability to correctly assign a frame to a robot when many robots are launched.
I'm still wondering about the issue of sensor_name
, because while in the case of a real robot there should be no problems, in the simulation you need to create sensor objects with different namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello!
I think it is rather very important, because the sensor readings should correspond to a specific frame on the robot and this might be important. Regarding multi robot i agree that we would need this separation, then we can use the namespace, but why tf_prefix?. Moreover, i believe we have to make sure that it is a valid link.
Can you explain to me the instances that's a requirement for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are asking specifically about tf_prefix, I don't need it, but I added it to unify arg in the packages.
As for checking whether a frame exists, it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about the problem with sensor_name
? The simplest solution is to add a parameter that adds a namespace to sensor_name
or add a sensor name prefix.
Generally, what I care about is being able to run many robots in simulation and on hardware while making a minimum number of changes to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafal-gorecki ok now I got the use-case. Yes, it is tricky to make it work with simulation with this namespace. What can be done is probably, instead of using the tf_prefix
, just add it to the sensor_name
already. I'm not sure how to automatically configure it based on the robot. The current changes however, doesn't fix it right?
Any ideas on how to solve it are welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, so much for today. I will try to add this functionality soon. I would like to ask you to confirm whether this tf_prefix can be left for imu in order to standardize the packages, or should I get rid of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss your idea on how you are planning to tackle the idea of sensor name automatically, and then it's better to start changing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The best solution is probably to add some kind of standard that make
robot_state_publisher
with namespace add to alllink
reference_frame
andsensor topic
namespaces. - Or add
namespace
argument to URDF and put it in all resonable field.
Then add the argument add_namespace_to_sensor_name
(bool)
@saikishor what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second solution added
Related with #1102, rabase to master.